Skip to content

SAK-51196 webcomponents Run tests on maven build #13489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 9, 2025

Conversation

adrianfish
Copy link
Contributor

@adrianfish adrianfish marked this pull request as draft March 25, 2025 22:41
@adrianfish adrianfish force-pushed the SAK-51196 branch 4 times, most recently from a01a2c6 to ee92c95 Compare March 26, 2025 13:15
@ottenhoff
Copy link
Contributor

cool!!!

@adrianfish adrianfish force-pushed the SAK-51196 branch 3 times, most recently from 194acb5 to 6881dff Compare March 28, 2025 18:34
@adrianfish adrianfish marked this pull request as ready for review March 28, 2025 18:35
@adrianfish
Copy link
Contributor Author

@ern @ottenhoff I think we should merge this into 25.x. I've fixed all the unit tests and made some a11y fixes that the tests caught.

@ottenhoff
Copy link
Contributor

agree i like it

@adrianfish
Copy link
Contributor Author

This needs to go in before anybody else adds significant webcomponents PRs.

@ern
Copy link
Contributor

ern commented Mar 31, 2025

strange that the cypress build fails but the other 2 maven builds pass!

@ottenhoff
Copy link
Contributor

remove the since origin/master part of lerna test

@adrianfish
Copy link
Contributor Author

@ottenhoff Are you sure about that? that switch is pretty nice. It tests the modified files and all the packages that depend on it. Do we want to test everything everytime?

@ottenhoff
Copy link
Contributor

@ottenhoff Are you sure about that? that switch is pretty nice. It tests the modified files and all the packages that depend on it. Do we want to test everything everytime?

Yes, I think it's the job of CI to just test everything. Locally it makes sense if you want a quick test

@adrianfish adrianfish force-pushed the SAK-51196 branch 6 times, most recently from fcafd03 to 62c0d9e Compare April 4, 2025 13:01
@adrianfish
Copy link
Contributor Author

Okay, this stuff's working now. It uses Playwright to download Chromium and runs the tests in parallel. Takes about 30 seconds. If you're working on a particular component, you can still run tests just for that that component and its dependant packages. Best of both world's hopefully.

@ottenhoff
Copy link
Contributor

This PR is consistently failing an Assignments cypress test in Grader.

http://localhost:8080/direct/assignment/setGrade.json POST returns a 400 when setting a letter grade on a student assignment

@adrianfish
Copy link
Contributor Author

adrianfish commented Apr 7, 2025

I am totally amiss as to why this would fail the cypress tests. The grader source is touched, but very superficially, just removing some "same-origin" options from the fetch requests (default anyway) and uncoupling the grader from the portal object a bit by using getSiteId instead of portal.siteId. This PR also does not touch AssignmentEntityProvider at all.

I just did a full master rebuild and ran the latest cypress tests, and it fails, so whatever I've done in this PR, the Cypress tests were already failing

@ottenhoff
Copy link
Contributor

you dont see a stacktrace locally when setting a letter grade on a student in Assignments?

@adrianfish
Copy link
Contributor Author

@ottenhoff Do you mean in the cypress output, or when actually directly using Sakai?

@ottenhoff
Copy link
Contributor

just replicate the cypress test: create an assignment using letter grades and try to grade a student in the Grader

@adrianfish
Copy link
Contributor Author

@ottenhoff works fine for me.

@ottenhoff
Copy link
Contributor

ottenhoff commented Apr 8, 2025

Easy to replicate the issue:

08-Apr-2025 10:58:12.965 WARN [http-nio-9080-exec-6] o.s.a.e.AssignmentEntityProvider.<init> The property "prop_new_assignment_add_to_gradebook" is null for the assignment feed
08-Apr-2025 10:58:13.281 INFO [http-nio-9080-exec-2] o.s.e.u.s.DirectServlet.dispatch Could not process entity: /assignment (400)[org.sakaiproject.entitybroker.exception.EntityException: You need to supply the courseId, gradableId, studentId and grade]: You need to supply the courseId, gradableId, studentId and grade (rethrown)

getSiteId is coming back empty inside the cypress test

@ottenhoff
Copy link
Contributor

export const getUserId = () => window.top?.portal?.user?.id || "";
export const getSiteId = () => window.top?.portal?.siteId || "";

I think you're basically saying here that Sakai MUST be the top window and absolutely can't be framed. this is a change from the previous use of portal.siteId

@adrianfish
Copy link
Contributor Author

adrianfish commented Apr 8, 2025

@ottenhoff They're all the same thing. top is safe because it will always refer to the topmost window in the current hierarchy, which might just be window, or it might be window.parent.

Ah, hang on. I get you. I just came back from the pub, so the noggin is on the go slow.

so, window?.portal?.siteId || window?.top?.portal?.siteId || "" would do the trick?

https://sakaiproject.atlassian.net/browse/SAK-51196

This change adds test scripts to the top level package.json that enable
us to test all the components at once using Playwright, and test just
the component which we are working on and its dependants. The full test
runs the tests in parallel and takes around 30s to run. The test:dev
script uses Lerna and will only test files changed since origin/master.
The tool pom runs the full component Playwright tests.
@adrianfish
Copy link
Contributor Author

@ottenhoff I still can't reproduce that error with letter grades. I've rebuild all the code, twice now, and still no joy.

@adrianfish
Copy link
Contributor Author

@ottenhoff Cypress passes now :) Was it because all the cypress stuff runs in frames? The only significant change in this PR was moving to getSiteId which would have tried the topmost window.

@ottenhoff
Copy link
Contributor

ottenhoff commented Apr 9, 2025 via email

@adrianfish
Copy link
Contributor Author

We obviously don't run any dependencies on portal-utils in frames at the moment. I'm glad Cypress caught that.

@adrianfish
Copy link
Contributor Author

@ottenhoff Can you approve it? Are we happy to go forward? If it causes issues with builds I can just take it out of the pom with another JIRA, but it seems pretty good now. 30 seconds of ui tests, plus the linting etc.

@adrianfish adrianfish merged commit 255baaf into sakaiproject:master Apr 9, 2025
4 checks passed
@adrianfish adrianfish deleted the SAK-51196 branch April 9, 2025 13:33
ern pushed a commit that referenced this pull request Apr 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants